src/pages/selection-plans/selection-plan-list-page.js (2)

88-95: 💤 Low value

Edit flow goes through URL push — works but worth a brief inline comment.

Clicking edit pushes /selection-plans/:id and relies on the routeSelectionPlanId effect to open the dialog. This is a nice property for deep-linking, but it's non-obvious and easy to break in future refactors. A one-line comment explaining the URL ↔ modal relationship would help.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/pages/selection-plans/selection-plan-list-page.js` around lines 88 - 95,
Add a one-line inline comment in the handleEdit function explaining that calling
history.push(`/app/summits/${currentSummit.id}/selection-plans/${selectionPlanId}`)
intentionally updates the URL to trigger the routeSelectionPlanId effect which
opens the edit modal/dialog (enables deep-linking), so future refactors know the
URL change is relied upon to show the modal rather than directly invoking a
modal open method.

---

60-73: ⚡ Quick win

Add a rejection handler to openEditModal.

If getSelectionPlan or getMarketingSettingsBySelectionPlan rejects (e.g., 404 on a deep-linked stale ID, network failure), the popup never opens and the user is left on the list with no signal. A .catch ensures the route-driven open flow degrades gracefully.

🛡️ Proposed fix

   const openEditModal = (selectionPlanId) => {
     if (!selectionPlanId) return;

     getSelectionPlan(selectionPlanId)
       .then(() =>
         getMarketingSettingsBySelectionPlan(
           selectionPlanId,
           null,
           DEFAULT_CURRENT_PAGE,
           MAX_PER_PAGE
         )
       )
-      .then(() => setOpenSelectionPlanPopup(true));
+      .then(() => setOpenSelectionPlanPopup(true))
+      .catch(() => {
+        if (routeSelectionPlanId) {
+          history.replace(`/app/summits/${currentSummit.id}/selection-plans`);
+        }
+      });
   };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/pages/selection-plans/selection-plan-list-page.js` around lines 60 - 73,
openEditModal's promise chain currently has no rejection handler so failures
from getSelectionPlan or getMarketingSettingsBySelectionPlan leave the UI
unchanged; add a .catch to the chain that handles errors (log/show a
notification and/or update error state) and still calls
setOpenSelectionPlanPopup(true) so the modal opens in a degraded state; keep
references to openEditModal, getSelectionPlan,
getMarketingSettingsBySelectionPlan and setOpenSelectionPlanPopup when
implementing the catch behavior.

src/layouts/selection-plan-layout.js (1)

41-52: 💤 Low value

LGTM, with a small readability note.

Routing is correct: the exact strict route on :selection_plan_id(\\d+) renders the list page (which auto-opens the edit dialog from the URL), while the non-exact one continues to delegate sub-paths (/extra-questions, /rating-types) to SelectionPlanIdLayout. Two <Route> entries with the same path is unusual at first glance — a brief inline comment ("exact → list with auto-open dialog; non-exact → nested sub-pages") would save future readers a double-take.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/layouts/selection-plan-layout.js` around lines 41 - 52, Add a brief
inline comment above the two Route entries explaining their intent: the
exact/strict Route with path `${match.url}/:selection_plan_id(\\d+)` renders
SelectionPlanListPage (which auto-opens the edit dialog from the URL), while the
non-exact Route with the same path delegates nested sub-pages to
SelectionPlanIdLayout (e.g., `/extra-questions`, `/rating-types`); place the
comment near the two Route elements so future readers immediately understand why
both routes share the same path.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/pages/selection-plans/selection-plan-list-page.js (1)

96-100: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Fix handleDelete to accept the row id, not a row object.

Line 97 returns early for the value MuiTable actually sends to onDelete, so delete clicks become no-ops here.

Suggested fix

-  const handleDelete = (selectionPlan) => {
-    if (!selectionPlan?.id) return;
-
-    deleteSelectionPlan(selectionPlan.id).then(() => refreshSelectionPlans());
+  const handleDelete = (selectionPlanId) => {
+    if (!selectionPlanId) return;
+
+    deleteSelectionPlan(selectionPlanId).then(() => refreshSelectionPlans());
   };

Based on learnings: When using MuiTable from openstack-uicore-foundation/lib/components/mui/table in this codebase, onDelete is called with the primitive row identifier, not the full row object.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/pages/selection-plans/selection-plan-list-page.js` around lines 96 - 100,
The handleDelete function currently expects a selectionPlan object and guards on
selectionPlan?.id, but MuiTable's onDelete passes the primitive row id instead
of the row object; update handleDelete to accept an id parameter (e.g., id) and
return early if id is falsy, then call deleteSelectionPlan(id).then(() =>
refreshSelectionPlans()); adjust any references that call handleDelete to ensure
they pass the id directly; keep function name handleDelete and preserve use of
deleteSelectionPlan and refreshSelectionPlans for locating the code.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/components/forms/selection-plan-form.js (1)

145-151: 💤 Low value

Consider guarding against undefined marketing_settings.

The code assumes marketing_settings exists when calling hasOwnProperty, but the render code uses optional chaining (?.) when reading these values. If propsEntity.marketing_settings is ever undefined, this will throw a TypeError. The same pattern appears in handleOnSwitchChange.

♻️ Suggested defensive guard

     if (id.startsWith("cfp_")) {
+      if (!newEntity.marketing_settings) {
+        newEntity.marketing_settings = {};
+      }
       if (
         !Object.prototype.hasOwnProperty.call(newEntity.marketing_settings, id)
       ) {

Apply similar guard in handleOnSwitchChange around line 247.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/forms/selection-plan-form.js` around lines 145 - 151, The code
assumes newEntity.marketing_settings exists before calling
Object.prototype.hasOwnProperty.call and before assigning keys; add a defensive
guard to initialize marketing_settings when missing (e.g., if
(!newEntity.marketing_settings) newEntity.marketing_settings = {}) before the
block that checks id.startsWith("cfp_") and likewise in handleOnSwitchChange so
both the hasOwnProperty call and subsequent assignment to
newEntity.marketing_settings[id].value are safe when
propsEntity.marketing_settings is undefined; update references to
newEntity.marketing_settings in those functions (the id.startsWith("cfp_")
branch and the handleOnSwitchChange handler) to rely on the initialized object.

src/components/forms/__tests__/selection-plan-form.test.js (1)

150-172: ⚡ Quick win

Consider asserting cleanup after successful save.

Test 3 verifies that onSavingChange(false) is called after rejection, but this test does not verify the same cleanup happens after success. According to the implementation in context snippet 1, .finally() always calls onSavingChange(false), so asserting that behavior here would make the test more complete.

✨ Suggested enhancement

    await act(async () => {
      resolveSave({ id: 1 });
      await Promise.resolve();
    });
+
+   await waitFor(() => {
+     expect(onSavingChange).toHaveBeenCalledWith(false);
+   });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/forms/__tests__/selection-plan-form.test.js` around lines 150
- 172, Add an assertion that the component runs its cleanup after a successful
save: after resolving the mocked onSubmit promise (resolveSave) in the test
"disables submit and calls onSavingChange(true) while save is pending", assert
that onSavingChange was later called with false and that the submit button is
enabled again (verifying the .finally() cleanup path implemented around
onSubmit/onSavingChange). Reference the mocked onSubmit, the onSavingChange spy,
and resolveSave when adding these assertions.

src/components/forms/__tests__/selection-plan-form.test.js (2)

94-98: 💤 Low value

Add time_zone_id to currentSummit for test robustness.

The form uses currentSummit.time_zone_id for date normalization in buildInitialValues and handleFormikSubmit. Tests pass because all date fields are null, but if any test later sets date values, moment.tz(value, undefined) will behave unexpectedly.

 const defaultProps = {
   entity: defaultEntity,
   errors: {},
-  currentSummit: { id: 1 },
+  currentSummit: { id: 1, time_zone_id: "UTC" },

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/forms/__tests__/selection-plan-form.test.js` around lines 94 -
98, In the defaultProps object, add the missing time_zone_id property to the
currentSummit object. Set it to a valid timezone string value (such as "UTC" or
another appropriate timezone identifier) so that when the form's
buildInitialValues and handleFormikSubmit methods use currentSummit.time_zone_id
for date normalization with moment.tz(), the timezone will be properly defined
rather than undefined.

---

18-68: 💤 Low value

Incomplete mocks for form's direct imports.

The form imports components from specific subpaths (e.g., openstack-uicore-foundation/lib/components/inputs/text-input, openstack-uicore-foundation/lib/components/mui/formik-inputs/datepicker) but the mock on line 18 targets the parent /lib/components path. Jest doesn't automatically mock subpaths.

Missing mocks for: MuiFormikDatepicker, Input (text-input), Panel, Dropdown, TextEditorV3, SortableTable (mui), SimpleLinkList.

Tests may pass now because these components render without throwing, but this creates fragility—if any component adds required context or validation, tests will fail unexpectedly.

🧪 Add targeted mocks for imported components

+jest.mock(
+  "openstack-uicore-foundation/lib/components/inputs/text-input",
+  () => ({
+    __esModule: true,
+    default: ({ id, value, onChange }) => (
+      <input id={id} value={value ?? ""} onChange={onChange} />
+    )
+  })
+);
+
+jest.mock(
+  "openstack-uicore-foundation/lib/components/mui/formik-inputs/datepicker",
+  () => ({
+    __esModule: true,
+    default: () => null
+  })
+);
+
+jest.mock(
+  "openstack-uicore-foundation/lib/components/sections/panel",
+  () => ({
+    __esModule: true,
+    default: ({ children }) => <div>{children}</div>
+  })
+);
+
+jest.mock(
+  "openstack-uicore-foundation/lib/components/inputs/dropdown",
+  () => ({
+    __esModule: true,
+    default: () => null
+  })
+);
+
+jest.mock(
+  "openstack-uicore-foundation/lib/components/inputs/editor-input-v3",
+  () => ({
+    __esModule: true,
+    default: () => null
+  })
+);
+
+jest.mock(
+  "openstack-uicore-foundation/lib/components/mui/sortable-table",
+  () => ({
+    __esModule: true,
+    default: () => null
+  })
+);
+
+jest.mock(
+  "openstack-uicore-foundation/lib/components/simple-link-list",
+  () => ({
+    __esModule: true,
+    default: () => null
+  })
+);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/forms/__tests__/selection-plan-form.test.js` around lines 18 -
68, The current mocks only target the parent path
`openstack-uicore-foundation/lib/components` but the form imports components
from specific subpaths like
`openstack-uicore-foundation/lib/components/inputs/text-input` and
`openstack-uicore-foundation/lib/components/mui/formik-inputs/datepicker`. Add
individual jest.mock() calls for each specific subpath that the
selection-plan-form component imports (including MuiFormikDatepicker, Input from
text-input, Panel, Dropdown, TextEditorV3, SortableTable from mui, and
SimpleLinkList), ensuring each mock properly exports the component as either a
default export or named export matching what the component expects.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes